Skip to content

Nopos-warn in vulpix & Move warn tests form tests/neg to tests/warn: Batch 1 #19242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 11, 2024
Merged

Nopos-warn in vulpix & Move warn tests form tests/neg to tests/warn: Batch 1 #19242

merged 1 commit into from
Jan 11, 2024

Conversation

szymon-rd
Copy link
Contributor

@szymon-rd szymon-rd commented Dec 11, 2023 •
edited
Loading

Adds functionality to vulpix that allows putting nopos-warns in comments & First batch of tests moved from tests/neg to tests/warn.
PR 2/5 (merge consecutively, per Nicolas' suggestion

Split up version of #18829

@szymon-rd szymon-rd changed the title Move warn tests form tests/error to tests/warn: Batch 1 Move warn tests form tests/neg to tests/warn: Batch 1 Dec 11, 2023
@szymon-rd szymon-rd force-pushed the move-error-warn-b1 branch 2 times, most recently from ec5ac6d to 9858d79 Compare December 12, 2023 14:08
@szymon-rd szymon-rd changed the title Move warn tests form tests/neg to tests/warn: Batch 1 Nopos-warn in vulpix & Move warn tests form tests/neg to tests/warn: Batch 1 Dec 13, 2023
@nicolasstucki
Copy link
Contributor

@szymon-rd you should not create branches on the dotty repo. We are executing all tests twice on this PR (push/pull). That is one of the reasons why we use the dotty-staging repo.

nicolasstucki
nicolasstucki reviewed Dec 19, 2023
View reviewed changes
Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

tests/warn/i3324.scala Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests/neg-deep-subtype/i3324.scala should be removed.

tests/warn/i5013.scala
@@ -0,0 +1,11 @@


class Foo {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For files where git cannot track the change, we might just remove the empty lines at the start. However, this could cause some additional check files to not be tracked.

Copy link
Contributor Author

@szymon-rd szymon-rd Dec 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could cause some additional check files to not be tracked

What do you mean?
But I can do that, it's a minor cosmetic issue so I didn't pay much attention to it, but it may be better to follow some formatting rules when it doesn't impose much cost.

@nicolasstucki
Copy link
Contributor

Should be squashed and rebased.

@szymon-rd
Copy link
Contributor Author

@nicolasstucki thank you for the review! however, could you take a look at: #19241 ? It's the first PR. This one is the second one.

Base automatically changed from warnings-in-vulpix to main December 21, 2023 21:01
@szymon-rd
Copy link
Contributor Author

@nicolasstucki I will squash it on merge, I removed the test.

@nicolasstucki
Copy link
Contributor

I will squash it on merge

Do you mean click in the squash and merge button?

@szymon-rd
Copy link
Contributor Author

Do you mean click in the squash and merge button?

Yes

@nicolasstucki
Copy link
Contributor

Do you mean click in the squash and merge button?

Yes

One of the points of squashing is to have a clean commit message. That button concatenates the commit messages, which defeats the purpose.

nicolasstucki
nicolasstucki approved these changes Jan 10, 2024
View reviewed changes
Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just needs a squash, rebase, and CI test again.

@szymon-rd
Copy link
Contributor Author

One of the points of squashing is to have a clean commit message. That button concatenates the commit messages, which defeats the purpose.

I usually have pretty good experience with this approach - The PR title is kept as a git commit title (first paragraph), and the commits are put in the commit description (next paragraphs). GitHub and other tools I have used render it nicely. But I agree it's better to squash it now, as we have to rebase and CI test it again.

@nicolasstucki
Copy link
Contributor

There was a conflict with tests/warn/i15474.scala

@szymon-rd szymon-rd merged commit 36831b8 into main Jan 11, 2024
@szymon-rd szymon-rd deleted the move-error-warn-b1 branch January 11, 2024 12:12
@Kordyjan Kordyjan added this to the 3.4.1 milestone Feb 14, 2024
@WojciechMazur WojciechMazur mentioned this pull request Jun 27, 2024
Merged
WojciechMazur added a commit that referenced this pull request Jun 28, 2024
@WojciechMazur
Backport "Nopos-warn in vulpix & Move warn tests form tests/neg to te…
e3c5e66 
…sts/warn: Batch 1" to LTS (#20821)

Backports #19242 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolasstucki nicolasstucki nicolasstucki approved these changes

Assignees

@szymon-rd szymon-rd

Labels
None yet
Projects
None yet
Milestone
3.4.1
Development

Successfully merging this pull request may close these issues.
3 participants
@szymon-rd @nicolasstucki @Kordyjan